feat: ability to pass in KFP token via env var.#35
feat: ability to pass in KFP token via env var.#35dmaniloff merged 5 commits intotrustyai-explainability:mainfrom
Conversation
…token retrieval in RagasEvaluatorRemote.
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/llama_stack_provider_ragas/remote/ragas_remote_eval.py:120` </location>
<code_context>
- load_kube_config(client_configuration=config)
- token = str(config.api_key["authorization"].split(" ")[-1])
+ kube_config = _load_kube_config()
+ token = str(kube_config.api_key["authorization"].split(" ")[-1])
except ImportError as e:
raise RagasEvaluationError(
</code_context>
<issue_to_address>
**issue (bug_risk):** Splitting the authorization header may be brittle if the format changes.
The code relies on a fixed 'Bearer <token>' format, which may not always be present. Please add validation or error handling to manage unexpected or missing formats.
</issue_to_address>
### Comment 2
<location> `src/llama_stack_provider_ragas/config.py:115-112` </location>
<code_context>
+ default=None,
+ )
+
+ kube_token: str | None = Field(
+ description=(
+ "Kubernetes token for Kubeflow pipeline execution. ",
+ "If not provided via env var, the token will be read from the local kubeconfig file.",
),
default=None,
)
</code_context>
<issue_to_address>
**nitpick:** The description field is a tuple, which may not render as intended.
Pydantic expects the description to be a single string. Please join the tuple elements into one string to ensure proper rendering.
</issue_to_address>
### Comment 3
<location> `src/llama_stack_provider_ragas/remote/kubeflow/utils.py:12-14` </location>
<code_context>
- logger = logging.getLogger(__name__)
- logger.setLevel(logging.INFO)
-
- try:
- config.load_incluster_config()
- except config.ConfigException:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Catching ConfigException without handling other exceptions may miss other errors.
Other exceptions, such as file not found or permission errors, may also occur when loading kubeconfig. Consider handling these cases to improve robustness.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| load_kube_config(client_configuration=config) | ||
| token = str(config.api_key["authorization"].split(" ")[-1]) | ||
| kube_config = _load_kube_config() | ||
| token = str(kube_config.api_key["authorization"].split(" ")[-1]) |
There was a problem hiding this comment.
issue (bug_risk): Splitting the authorization header may be brittle if the format changes.
The code relies on a fixed 'Bearer ' format, which may not always be present. Please add validation or error handling to manage unexpected or missing formats.
| try: | ||
| config.load_incluster_config(client_configuration=kube_config) | ||
| logger.info("Loaded in-cluster Kubernetes configuration") |
There was a problem hiding this comment.
suggestion (bug_risk): Catching ConfigException without handling other exceptions may miss other errors.
Other exceptions, such as file not found or permission errors, may also occur when loading kubeconfig. Consider handling these cases to improve robustness.
| import logging | ||
| import os | ||
|
|
||
| from kubernetes import client, config | ||
| from kubernetes.client.exceptions import ApiException | ||
|
|
||
| from llama_stack_provider_ragas.constants import ( | ||
| DEFAULT_RAGAS_PROVIDER_IMAGE, | ||
| KUBEFLOW_CANDIDATE_NAMESPACES, | ||
| RAGAS_PROVIDER_IMAGE_CONFIGMAP_KEY, | ||
| RAGAS_PROVIDER_IMAGE_CONFIGMAP_NAME, | ||
| ) |
There was a problem hiding this comment.
these imports were unnecessarily inside the scope of the function making it look like a kfp component but it is actually not.
adolfo-ab
left a comment
There was a problem hiding this comment.
lgtm as a temporary solution, but I think long-term we need RBAC (not sure if through the TrustyAI operator or through LLS, though)
…beflow configuration.
Add
KUBEFLOW_PIPELINES_TOKENenv var to the config so users can pass that in manually.